Skip to content

THV-0057: Address review feedback on rate limiting RFC#66

Merged
jerm-dro merged 1 commit intomainfrom
jerm/thv-0057-review-responses
Apr 4, 2026
Merged

THV-0057: Address review feedback on rate limiting RFC#66
jerm-dro merged 1 commit intomainfrom
jerm/thv-0057-review-responses

Conversation

@jerm-dro
Copy link
Copy Markdown
Contributor

@jerm-dro jerm-dro commented Apr 3, 2026

Summary

Addresses all 18 review comments from @ChrisJBurns on THV-0057 (Rate Limiting for MCP Servers).

Key changes:

  • Middleware ordering: Updated diagram to show Auth → MCP Parser → RateLimit → [remaining chain]
  • Config renames: capacitymaxTokens, refillPeriodSecondsrefillPeriod (Duration type)
  • Admission-time validation: CRD schema validation instead of startup errors, RateLimitingConfigValid status condition
  • vMCP field placement: Moved from spec.config.rateLimiting to spec.rateLimiting for consistent CRD validation
  • Redis keys: Added namespace to prevent cross-namespace collisions, specified TTL and key derivation
  • Optimizer interaction: Documented as tech debt, links to THV-0060 and Enforce Cedar policies on optimizer find_tool and call_tool toolhive#4385
  • JSON-RPC error format: Concrete rejection response with error code -32029 and Retry-After in error.data
  • Observability: Added metric categories and span attributes
  • Fail-open: State-transition logging, Sentinel failover bucket reset documented
  • Security: TLS contradiction fixed, scope and limitations section added
  • Alternatives: Added Envoy/Istio, webhook-based, and fixed window counter evaluations
  • Plan section: MCPRemoteProxy and CLI rate limiting deferred

Test plan

  • Review RFC changes match the posted PR comment responses

🤖 Generated with Claude Code

Updates include: middleware ordering (auth → mcp-parser → rate limit),
config field renames (maxTokens, refillPeriod as Duration), admission-time
validation, namespace in Redis keys, vMCP field placement at spec level,
optimizer interaction as tech debt, JSON-RPC error format, observability
metrics, fail-open semantics, and additional alternatives considered.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@jerm-dro jerm-dro requested a review from ChrisJBurns April 3, 2026 23:38
@jerm-dro jerm-dro merged commit 7c3a40c into main Apr 4, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants